AMM-1456 : skiped jwttoken validation if not present from mobile #49
Conversation
WalkthroughThe JWT validation filter was refactored to independently validate JWT tokens from cookies and headers, instead of selecting one source. If both validations fail, the filter checks for mobile clients and allows them if an authorization header is present. A new helper method for mobile client detection was added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Filter
participant Chain
Client->>Filter: Send HTTP Request (with Cookies/Headers)
alt JWT token in Cookie is valid
Filter->>Chain: Proceed with request
else JWT token in Header is valid
Filter->>Chain: Proceed with request
else Request from Mobile Client with Authorization Header
Filter->>Chain: Proceed with request
else
Filter-->>Client: Respond with 401 Unauthorized
end
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/main/java/com/iemr/helpline104/utils/JwtUserIdValidationFilter.java (1)
53-56: 🛠️ Refactor suggestion
⚠️ Potential issueAvoid logging raw JWTs & drop the unused variable
jwtTokenFromHeaderis created only for this log statement and never used later.
More importantly, printing the raw token to logs is a direct PII / security leak: log files can be scraped and tokens replayed.-String jwtTokenFromHeader = request.getHeader("Jwttoken"); -logger.info("JWT token from header: " + jwtTokenFromHeader); +// Fetch the header only once, downstream – and never print the full value. +// logger.debug("JWT token present in header: {}", jwtTokenFromHeader != null);[action] Delete the variable or move the retrieval to the main validation block and never log the literal value.
🧹 Nitpick comments (2)
src/main/java/com/iemr/helpline104/utils/JwtUserIdValidationFilter.java (2)
92-96: Mobile-bypass path needs stronger guaranteesAt present any request with a
User-Agentcontaining “okhttp” and any non-nullAuthorizationheader is let through with zero validation.
Attackers can trivially spoof both headers:User-Agent: okhttp/4.9.3 Authorization: garbageRecommendations:
- Verify that the
Authorizationheader actually carries a token you expect (e.g.,Bearer …) and validate it.- Consider whitelisting known mobile app fingerprints instead of a substring match.
- Log (without secrets) whenever this path is used so anomalies can be audited.
105-110:isMobileClientdetection is brittle & hard-codedThe helper now flags a request as “mobile” only if the user-agent contains
"okhttp".
This:
- Misses legitimate Android/iOS clients not using OkHttp.
- Allows easy header spoofing.
Consider a more robust strategy:
-return userAgent.contains("okhttp"); +return userAgent.matches(".*(okhttp|android|ios|iphone|ipad).*");or, better, rely on an explicit custom header set by your mobile apps.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
src/main/java/com/iemr/helpline104/utils/JwtUserIdValidationFilter.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (java)
| String jwtFromCookie = getJwtTokenFromCookies(request); | ||
| String jwtFromHeader = request.getHeader("JwtToken"); | ||
| String authHeader = request.getHeader("Authorization"); | ||
|
|
||
| if (jwtFromCookie != null) { | ||
| logger.info("Validating JWT token from cookie"); | ||
| if (jwtAuthenticationUtil.validateUserIdAndJwtToken(jwtFromCookie)) { | ||
| filterChain.doFilter(servletRequest, servletResponse); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (jwtFromHeader != null) { | ||
| logger.info("Validating JWT token from header"); | ||
| if (jwtAuthenticationUtil.validateUserIdAndJwtToken(jwtFromHeader)) { | ||
| filterChain.doFilter(servletRequest, servletResponse); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Header name mismatch silently breaks authentication
You first read "Jwttoken" (line 54) and later read "JwtToken" (line 71).
Many servlet containers treat headers as case-insensitive but the spelling must still match on the client side. If clients send "Jwttoken" (as before), the second read will return null, causing the header path to be skipped and – depending on the user-agent – the request may be granted without validation.
-String jwtFromHeader = request.getHeader("JwtToken");
+// Use a single, canonical header name through the whole filter
+final String JWT_HEADER = "Jwttoken";
+String jwtFromHeader = request.getHeader(JWT_HEADER);Also consider turning the cookie/header names into private static final constants to prevent this kind of drift.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String jwtFromCookie = getJwtTokenFromCookies(request); | |
| String jwtFromHeader = request.getHeader("JwtToken"); | |
| String authHeader = request.getHeader("Authorization"); | |
| if (jwtFromCookie != null) { | |
| logger.info("Validating JWT token from cookie"); | |
| if (jwtAuthenticationUtil.validateUserIdAndJwtToken(jwtFromCookie)) { | |
| filterChain.doFilter(servletRequest, servletResponse); | |
| return; | |
| } | |
| } | |
| if (jwtFromHeader != null) { | |
| logger.info("Validating JWT token from header"); | |
| if (jwtAuthenticationUtil.validateUserIdAndJwtToken(jwtFromHeader)) { | |
| filterChain.doFilter(servletRequest, servletResponse); | |
| return; | |
| } | |
| } | |
| String jwtFromCookie = getJwtTokenFromCookies(request); | |
| // Use a single, canonical header name through the whole filter | |
| final String JWT_HEADER = "Jwttoken"; | |
| String jwtFromHeader = request.getHeader(JWT_HEADER); | |
| String authHeader = request.getHeader("Authorization"); | |
| if (jwtFromCookie != null) { | |
| logger.info("Validating JWT token from cookie"); | |
| if (jwtAuthenticationUtil.validateUserIdAndJwtToken(jwtFromCookie)) { | |
| filterChain.doFilter(servletRequest, servletResponse); | |
| return; | |
| } | |
| } | |
| if (jwtFromHeader != null) { | |
| logger.info("Validating JWT token from header"); | |
| if (jwtAuthenticationUtil.validateUserIdAndJwtToken(jwtFromHeader)) { | |
| filterChain.doFilter(servletRequest, servletResponse); | |
| return; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/iemr/helpline104/utils/JwtUserIdValidationFilter.java
around lines 54 and 70-88, the header name "JwtToken" is inconsistently spelled
as "Jwttoken" and "JwtToken", causing the header read to fail silently and
potentially bypass authentication. To fix this, unify the header name spelling
to a single consistent value used both when reading and in client requests.
Additionally, define the cookie and header names as private static final
constants to avoid such mismatches in the future.
2 participants
📋 Description
JIRA ID:
Please provide a summary of the change and the motivation behind it. Include relevant context and details.
✅ Type of Change
ℹ️ Additional Information
Please describe how the changes were tested, and include any relevant screenshots, logs, or other information that provides additional context.
Summary by CodeRabbit